Fixes updates to controller's UnavailableSince field. #1246
Conversation
alesstimec
force-pushed
the
controller-unavailable-since-update
branch
from
4e4850d to
f0144ad
Compare
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
alesstimec
force-pushed
the
controller-unavailable-since-update
branch
from
f0144ad to
fecbb65
Compare
alesstimec
force-pushed
the
controller-unavailable-since-update
branch
from
fecbb65 to
cc8debc
Compare
internal/jimm/watcher_test.go
Outdated
| cerr := <-controllerUnavailableChannel | ||
| c.Assert(cerr, qt.IsNil) |
There was a problem hiding this comment.
I think it would be better to avoid the use of this channel in tests where we don't need to use them. Like here, we could just check the DB to verify that the UnavailableSince time is nil. It will also make it easier to refactor out the channel later.
There was a problem hiding this comment.
i think tests need this channel for timing..
There was a problem hiding this comment.
I believe timing is handled by the cancel() call inside the AllModelWatcherNext_ mock + the waitGroup, so you should be able to safely remove this. And it would be good to check the DB and the UnavailableSince field since that is what the test is trying to assert.
| wg.Add(1) | ||
| go func() { | ||
| defer wg.Done() | ||
| err := w.Watch(ctx, time.Millisecond) |
There was a problem hiding this comment.
Just a side note, our watcher tests might want to consider adding a deadline of e.g. 5 seconds to the context, that way we avoid the test getting stuck and running for too long when writing new tests/modifying old ones.
internal/jimm/watcher_test.go
Outdated
| cerr := <-controllerUnavailableChannel | ||
| c.Assert(cerr, qt.IsNil) |
There was a problem hiding this comment.
I believe timing is handled by the cancel() call inside the AllModelWatcherNext_ mock + the waitGroup, so you should be able to safely remove this. And it would be good to check the DB and the UnavailableSince field since that is what the test is trying to assert.
alesstimec
force-pushed
the
controller-unavailable-since-update
branch
2 times, most recently
from
e694741 to
869ec13
Compare
alesstimec
force-pushed
the
controller-unavailable-since-update
branch
from
869ec13 to
0b7b585
Compare
Description
We were never resetting controller's UnavailableSince field. So if JIMM was unable to dial the controller at some point, that controller would forever be shown as unavailable in
jimmctl controllersoutput.Engineering checklist
Check only items that apply
Test instructions
Notes for code reviewers